fix(payment gateway account): remove payment gateway field #50681
Conversation
📝 WalkthroughWalkthroughThis change refactors the Payment Gateway Account DocType to resolve a reference to a deleted Payment Gateway DocType. The hardcoded "payment_gateway" Link field is removed from the DocType JSON schema and replaced with a migration patch that conditionally creates it as a custom field when the payments application is installed. The payment_channel field type definition is also expanded to support an "Other" option. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
erpnext/patches/v16_0/create_payment_gateway_account_custom_fields.py (1)
5-22: Patch logic for conditional Custom Field creation looks solid; consider UX placement and late-install cases.The
executeimplementation correctly gates on thepaymentsapp, usescreate_custom_fieldsin the expected way, and clears the doctype cache, so it should be safe and idempotent for existing sites with payments installed.Two points to double‑check:
- If a site installs the
paymentsapp after this ERPNext patch has already run, this patch will not re‑execute, sopayment_gatewaywill not be auto‑created there. That may be fine given the stated goal ("if the payments app is already installed"), but it’s worth confirming thatpaymentsitself covers this scenario or that it’s explicitly out of scope.- Without an
insert_after, the new custom field will appear at the end of the form instead of where the core field used to sit. If you care about preserving the previous layout, you might want to add e.g."insert_after": "payment_channel"(or"company") to the field dict.erpnext/patches.txt (1)
452-452: Confirm ordering vsdelete_payment_gateway_doctypesand behavior across app combinations.Registering
erpnext.patches.v16_0.create_payment_gateway_account_custom_fieldsin thepost_model_syncblock, aftererpnext.patches.v15_0.delete_payment_gateway_doctypes, is consistent with usingcreate_custom_fields(schema is already in place) and ensures the new Custom Field is not seen by the delete patch.Given the history behind issue #45220, it would be good to explicitly confirm:
- On sites where
paymentsis not installed,delete_payment_gateway_doctypesbehaves as intended and this new patch remains a no-op.- On sites where
paymentsis installed,delete_payment_gateway_doctypesdoes not end up deleting thePayment GatewayDocType thatpaymentsowns, and the new custompayment_gatewaylink created here points at the correct surviving DocType.If this interaction is already covered by tests or by safeguards inside
delete_payment_gateway_doctypes, a brief note in the PR description or a comment in that patch might help future readers.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
erpnext/accounts/doctype/payment_gateway_account/payment_gateway_account.json(1 hunks)erpnext/accounts/doctype/payment_gateway_account/payment_gateway_account.py(1 hunks)erpnext/patches.txt(1 hunks)erpnext/patches/v16_0/create_payment_gateway_account_custom_fields.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
erpnext/patches/v16_0/create_payment_gateway_account_custom_fields.py (1)
erpnext/support/doctype/service_level_agreement/service_level_agreement.py (2)
create_custom_fields(288-309)clear_cache(252-254)
🪛 Ruff (0.14.5)
erpnext/accounts/doctype/payment_gateway_account/payment_gateway_account.py
23-23: Syntax error in forward annotation: Expected an expression
(F722)
23-23: Undefined name Email
(F821)
23-23: Undefined name Phone
(F821)
23-23: Undefined name Other
(F821)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Patch Test
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Summary
🔇 Additional comments (2)
erpnext/accounts/doctype/payment_gateway_account/payment_gateway_account.py (1)
23-23: Literal type update correctly tracks the new"Other"option; verify static-analysis behavior.Expanding the
payment_channelhint toDF.Literal["", "Email", "Phone", "Other"]matches the updated Select options in the DocType JSON and keeps type-checkers honest; the change is confined to theTYPE_CHECKINGblock so it has no runtime impact.Ruff’s F722/F821 hints here look like a quirk of how it parses the
DF.Literal[...]pattern rather than a real Python issue, but if CI is actually failing on these, you may need to either (a) adjust the Ruff configuration for this file/line, or (b) fall back to a looser annotation (e.g.DF.Data) for this one field.erpnext/accounts/doctype/payment_gateway_account/payment_gateway_account.json (1)
7-76: Corepayment_gatewayfield removal relies entirely on the new Custom Field patch; confirm behavior whenpaymentsis absent.The DocType JSON now no longer defines a core
payment_gatewayfield, which addresses the “deleted DocType is still referenced” issue and is consistent with re‑introducing it via a Custom Field only when thepaymentsapp is installed.Given that the Python controller (
PaymentGatewayAccount.autoname) still usesself.payment_gateway, please verify:
- For sites with
paymentsinstalled, the new patch reliably creates thepayment_gatewayCustom Field so existing logic and data continue to work.- For sites without
payments, it’s acceptable thatPayment Gateway Accountno longer exposespayment_gatewayat all and that autoname may see an empty value (i.e., this doctype isn’t expected to be used there).If you expect
Payment Gateway Accountto never be used withoutpayments, a brief comment in the controller or DocType description might help future maintainers understand this coupling.Also applies to: 80-80
1 participant
Removed the
payment_gatewayfield from the Payment Gateway Account DocType. Added a patch to add thepayment_gatewaycustom field if the payments app is already installed.Closes: #45220
Ref: frappe/payments#185